Skip to content

Fix for LT-22563#993

Merged
AndyBlack merged 6 commits into
mainfrom
LT22563
Jul 8, 2026
Merged

Fix for LT-22563#993
AndyBlack merged 6 commits into
mainfrom
LT22563

Conversation

@AndyBlack

@AndyBlack AndyBlack commented Jul 6, 2026 •
edited by jasonleenaylor
Loading

Copy link
Copy Markdown
Collaborator

This turned out to be a continuation of LT-22422 "XAmple can incorrectly report success for a variant referring to an entry with multiple MSA" The fix there took care of Try a Word, but not what fed into the parser filer. This fixes that.
The result from XAmple can include MSI elements like these:


which have three parts: the lex entry hvo, the lex ref hvo. and the msa hvo. We were ignoring the msa hov when passing the XAmple parsing result to the parser filer.

This change is Reviewable

@github-actions

github-actions Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   10m 21s ⏱️ +51s
4 302 tests +3  4 229 ✅ +3  73 💤 ±0  0 ❌ ±0 
4 311 runs  +3  4 238 ✅ +3  73 💤 ±0  0 ❌ ±0 

Results for commit f6e1099. ± Comparison against base commit b7c9ce5.

♻️ This comment has been updated with latest results.

johnml1135
johnml1135 reviewed Jul 7, 2026
View reviewed changes
Comment thread Src/LexText/ParserCore/XAmpleParser.cs
@johnml1135

johnml1135 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

General comment (not diff-anchored — this file isn't touched by this PR, so GitHub won't let me attach it to a line):

Src/Utilities/pcpatrflex/ToneParsFLExDll/ToneParsInvoker.cs, TryCreateParseMorph/ProcessMsaHvo (~line 799-891)

This class's TryCreateParseMorph/ProcessMsaHvo is explicitly labeled "from XAmpleParser.cs" (see the comment at line 729: "ConvertParserFilerResultXmlToParseResult(), TryCreateParseMorph(), and ProcessMsaHvo() are from XAmpleParser.cs ... Ideally, we'd expose them from XAmpleParser.cs"). It still has the old 2-tuple ProcessMsaHvo that discards the MSA hvo — the exact bug this PR fixes for XAmple. Since ToneParsInvoker is a live, separately-tested class (see ToneParsInvokerTests.cs), the PC-PATR/tone-parser path will still reproduce the "valid parse not in the list" symptom from LT-22563/LT-22422 for entries with multiple MSAs. Might be worth either applying the same fix there, or (per the existing TODO) finally exposing ProcessMsaHvo/TryCreateParseMorph from XAmpleParser.cs so there's one implementation.

@johnml1135

johnml1135 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

General comment (not diff-anchored — no test files are touched by this PR):

Missing test coverage for the fixed code path

Neither XAmpleParserTests.cs nor ParseFilerProcessingTests.cs exercises TryCreateParseMorph/ProcessMsaHvo against a 3-part MSI DbRef (e.g. 19938.0.14573) resolving to an IMoStemMsa. This is exactly the kind of format-parsing detail that regressed silently once already (LT-22422 fixed the writer side but not the parser-filer side, which is what led to LT-22563). A regression test mirroring the JIRA acceptance test — a variant LexEntry with multiple MSAs parsed through ParseWordXml/TryCreateParseMorph — would guard against this reappearing in a future refactor.

@johnml1135

johnml1135 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

All thee comments AI generated - human review forthcoming.

@AndyBlack

AndyBlack commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

General comment (not diff-anchored — this file isn't touched by this PR, so GitHub won't let me attach it to a line):

Src/Utilities/pcpatrflex/ToneParsFLExDll/ToneParsInvoker.cs, TryCreateParseMorph/ProcessMsaHvo (~line 799-891)

This class's TryCreateParseMorph/ProcessMsaHvo is explicitly labeled "from XAmpleParser.cs" (see the comment at line 729: "ConvertParserFilerResultXmlToParseResult(), TryCreateParseMorph(), and ProcessMsaHvo() are from XAmpleParser.cs ... Ideally, we'd expose them from XAmpleParser.cs"). It still has the old 2-tuple ProcessMsaHvo that discards the MSA hvo — the exact bug this PR fixes for XAmple. Since ToneParsInvoker is a live, separately-tested class (see ToneParsInvokerTests.cs), the PC-PATR/tone-parser path will still reproduce the "valid parse not in the list" symptom from LT-22563/LT-22422 for entries with multiple MSAs. Might be worth either applying the same fix there, or (per the existing TODO) finally exposing ProcessMsaHvo/TryCreateParseMorph from XAmpleParser.cs so there's one implementation.

I will create a separate JIRA issue for this.

@AndyBlack

AndyBlack commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Missing test coverage for the fixed code path
I wish I knew how to create unit tests for these kinds of things. I've never been able to figure out how to do it; hence, there aren't any. Do you have ideas on how it could be done?

@johnml1135

johnml1135 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Added a regression test for this in c350a58: ParseFilerProcessingTests.TryCreateParseMorph_IrregularVariantWithMultipleMsas_UsesMsaFromDbRef.

It builds an irregularly inflected variant entry ("creb") of a base entry ("seek") that has two MSAs, invokes the private XAmpleParser.TryCreateParseMorph (via reflection) with a hand-built <Morph><MoForm DbRef="..."/><MSI DbRef="lexEntryHvo.0.msaHvo"/></Morph> element pointing at the second MSA — mimicking what XAmple echoes back — and asserts the resulting ParseMorph.Msa is that exact second MSA, not the variant's first/main sense (which is what the pre-fix code would have produced via GetMainOrFirstSenseOfVariant). Confirmed it exercises the fixed path by tracing it against the still-unfixed duplicate of this logic in ToneParsInvoker.cs (flagged in an earlier comment), which would return the wrong (first) MSA for this same setup.

AndyBlack and others added 5 commits July 8, 2026 11:43
@AndyBlack @johnml1135
Fix for LT-22563
06f3644 
Change-Id: I60967f8053f07355f03c3b0a320791d1880d1822
@AndyBlack @johnml1135
Avoid duplicatre code for ProcessMsaHvo()
9980d41 
Change-Id: I2223a0b3b3be5d4831cb2d25c591dcf4f47a0afb
@AndyBlack @johnml1135
Fix comment in TryCreateParseMorph()
68a9a90 
Change-Id: Ida91b219515634f14ba83f594d226f68d5b16585
@johnml1135 @claude
Add regression test for LT-22563 MSI DbRef parsing
bd03e81 
TryCreateParseMorph must use the msa hvo encoded in the MSI DbRef
(lexEntryHvo.refIndex.msaHvo) for an irregularly inflected variant,
not just fall back to the variant's first/main sense, since that can
be the wrong MSA when the underlying entry has more than one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@johnml1135 @claude
Harden LT-22563 regression test with multiple entry refs
9ab9734 
Extends the DbRef-parsing test to cover a variant with two
EntryRefsOS, each targeting a different base entry that itself has
two MSAs. This checks both halves of the "lexEntryHvo.refIndex.msaHvo"
encoding: the refIndex picking the right LexEntryRef, and the msaHvo
picking the right MSA within it, not just the single-ref case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@johnml1135 @claude
Update stale case-4 doc comment for the 3-part MSI DbRef
f6e1099 
Case 4 in the TryCreateParseMorph doc comment still described the
pre-LT-22422 2-part "y.index" encoding. Update it to describe the
current 3-part "y.i.z" format (LexEntry hvo, LexEntryRef index, and
the specific MSA hvo), matching what ProcessMsaHvo actually parses.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
johnml1135
johnml1135 approved these changes Jul 8, 2026
View reviewed changes

@johnml1135 johnml1135 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just make sure the new JIRA issue is linked.

johnml1135
johnml1135 approved these changes Jul 8, 2026
View reviewed changes

@johnml1135 johnml1135 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnml1135 reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on AndyBlack).

@AndyBlack AndyBlack merged commit 22a5f7b into main Jul 8, 2026
6 of 7 checks passed
@AndyBlack AndyBlack deleted the LT22563 branch July 8, 2026 17:28
@johnml1135

johnml1135 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

New JIRA issue: https://jira.sil.org/browse/LT-22607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnml1135 johnml1135 johnml1135 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AndyBlack @johnml1135